-
Notifications
You must be signed in to change notification settings - Fork 30
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
gen, protos: 0.3, single cert #191
Conversation
Signed-off-by: William Woodruff <[email protected]>
@@ -32,8 +32,8 @@ option ruby_package = "Sigstore::Bundle::V1"; | |||
// The primary message ('Bundle') MUST be versioned, by populating the | |||
// 'media_type' field. Semver-ish (only major/minor versions) scheme MUST | |||
// be used. The current version as specified by this file is: | |||
// application/vnd.dev.sigstore.bundle+json;version=0.2 | |||
// The semantic version is thus '0.2'. | |||
// application/vnd.dev.sigstore.bundle+json;version=0.3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just pressure testing - Do we want a new version at this point? I think yes. We could add the field without a new version to avoid this being a breaking change, but then the field would have to be optional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I think so as well -- IMO making the field non-optional would be confusing in context, since the other two content
variants are marked as required.
(But maybe there are other good reasons to not bump the version here? Other client maintainers should probably chime in 🙂)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would think that a breaking change is desired if we want clients to actually adopt the usage of the new certificate
. If not, clients would need to write the certificate into both x509_certificate_chain
and certificate
, and I'm not sure that's desirable?
I think we can roll this out quite easy by describing the change more of a clarification, and as part of that moving the certificate to a new field and making it explicit. This would of course still require clients to discriminate on the version, and this is less optimal for verifiers that would need to be compliant with multiple versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would think that a breaking change is desired if we want clients to actually adopt the usage of the new
certificate
. If not, clients would need to write the certificate into bothx509_certificate_chain
andcertificate
, and I'm not sure that's desirable?
My 0.02c would be for encouraging clients to adopt the certificate
form -- the language in VerificationMaterial
currently says that adopting the 0.3
bundle for keyless signing + PGI means that clients MUST
use the certificate
field instead of x509_certificate_chain
.
But yeah, this does make things a bit of a mess for verifying clients...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For clients that wish to only support PGI, we could say just take the first cert of cert-chain
or certificate
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the current wording is good 👍 . My comment was a general about clients that would need to deal with multiple versions, not how the fields are used to day. I.e that regardless what we do, clients have to pay the price :) This is of course not specific to this spec, it's a general statement of concurrently supporting multiple versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, maybe it's time we plan for a next release, i.e bulk more things together as you mentioned. Don't think there is any urgency around getting v0.3
out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No extreme urgency, although I would definitely like to get #189 merged sooner rather than later 🙂 -- there's a lot of work that isn't strictly blocked on it, but will probably become a client interop challenge unless we eventually enumerate the various sigs/hashes that can occur on the PGI.
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
Co-authored-by: Fredrik Skogman <[email protected]> Signed-off-by: William Woodruff <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this 🚀
Signed-off-by: William Woodruff <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
In terms of procedure, should we merge here and let |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, merge this and let 0.3
live at main
until we cut a release.
This adds a new
oneof
variant toVerificationMaterials.contents
that specifically encodes a single X.509 leaf certificate. This ratchets down (soft) invariants that were previously documented on thex509_certificate_chain
member.In the process, it also bumps the bundle version (
0.3
) and moves some documentation around.Closes #190.